-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(MeshHTTPRoute): add basic validation #5625
feat(MeshHTTPRoute): add basic validation #5625
Conversation
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the tests
@jakubdyszkiewicz maybe that validation runs at all, but IMO there isn't validation complicated enough to write tests for yet. I'll just end up with a list of kinds in a test file that matches the declared list of kinds in the validation file. Might as well just make sure the one list is correct. |
I disagree. A restricted set of kinds on targetRef + to is an important part of the policy validation. I don't see why this unreasonable test. |
Right but how do you check that the test is correct in the first place? You need to read the list of kinds that are listed in the tests. But there's already a perfectly readable, declarative list of kinds in the validation code, you can just read that instead when reviewing. There's no point to such a test IMO, it's not actually any safer. |
d24cd2d
to
cba164f
Compare
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
cba164f
to
7f4137e
Compare
@jakubdyszkiewicz are there enough tests in this PR in your opinion? |
I wouldn't do it that way. I would go to MADR to check if there is an entry about which targetRefs are supported (maybe re-think if that entry is correct). Testing it is a bit much but because we have general tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok, one thing I noticed looking around the code is that weight is defined as "int" but I think it should be >0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also add an element in to
, but yes. It's enough. The helper is also pretty cool :)
I think we want to allow weights of 0 for turning off and on a backend but yeah actually it should be a uint |
Waiting on #5652 |
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
032748d
to
83c390e
Compare
I mean if you had exhaustive tests for the supported kinds, you'd need to read which ones are in the tests to make sure the tests are correct. Which ones are correct you'd know from reading the MADR.
Which is what I mean by skipping the exhaustive tests entirely |
Signed-off-by: Mike Beaumont <mjboamail@gmail.com> Signed-off-by: Bart Smykla <bartek@smykla.com>
Checklist prior to review
MeshHTTPRoute
basic api implementation #5470syscall.Mkfifo
have equivalent implementation on the other OS --UPGRADE.md
? --> Changelog:
entry here or add aci/
label to run fewer/more tests?